Improve async() by making its promises cancellable#20
Conversation
|
I think this following case would fail 🤔 $promise = async(function (): int {
echo 'a';
await(async(function(): void {
await(async(function(): void {
await(sleep(2));
})());
})());
echo 'b';
return time();
})(); |
WyriHaximus
force-pushed
the
cancel-fiber
branch
from
9e5259c to
77dac0c
Compare
WyriHaximus
force-pushed
the
cancel-fiber
branch
from
77dac0c to
83c90c2
Compare
WyriHaximus
force-pushed
the
cancel-fiber
branch
3 times, most recently
from
208a782 to
ab2e7b8
Compare
WyriHaximus
force-pushed
the
cancel-fiber
branch
5 times, most recently
from
794c7d8 to
1dd9138
Compare
WyriHaximus
changed the title
async() by making its promises cancelable
WyriHaximus
force-pushed
the
cancel-fiber
branch
2 times, most recently
from
d86b691 to
8a65d99
Compare
|
@WyriHaximus looks good to me, only thing I just noticed is that the description for cancellation is missing inside the |
WyriHaximus
force-pushed
the
cancel-fiber
branch
from
8a65d99 to
25810ab
Compare
@SimonFrings done 👍 |
WyriHaximus
force-pushed
the
cancel-fiber
branch
from
25810ab to
9dfeea4
Compare
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Some great progress in here, love this feature and how beautifully it aligns with our existing cancellation logic (#9 / #13)!
Added a number of smaller remarks below, otherwise looks like an excellent addition! Semantics make a lot of sense to me, so I'd love to get this in as is, but I do see some room for follow-up discussions once we address reactphp/promise#56 in the future 👍
WyriHaximus
force-pushed
the
cancel-fiber
branch
from
9dfeea4 to
24e5b93
Compare
|
@clue Yes this one is the missing link we need to make this package fully fit into our ecosystem. As mentioned on some of your comments, I will create a follow up PR to utilize WeakMap and WeakReference where possible to put the garbage collector in charge of cleaning up promise <-> fiber references. But for the sake of landing this feature, and requiring more time to fully test the behavior there I didn't update this PR with that. |
clue
changed the title
async() by making its promises cancelableasync() by making its promises cancellable
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Some good progress! I agree the WeakMap implementation can be left up to a future PR, only added a couple of remarks below, otherwise LGTM!
WyriHaximus
force-pushed
the
cancel-fiber
branch
3 times, most recently
from
9df8ce5 to
f400d20
Compare
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
WyriHaximus
force-pushed
the
cancel-fiber
branch
from
f400d20 to
262ef59
Compare
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Thanks for the update, changes LGTM, so let's get this shipped! 
6 participants
Since
async()returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceledsleep()call.This builds on top of #15, #18, #19, #26, #28, #30, and #32.